-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix extra stack frame on exception stack trace #99263
Conversation
@@ -771,7 +771,13 @@ private static void DispatchEx(scoped ref StackFrameIterator frameIter, ref ExIn | |||
|
|||
DebugScanCallFrame(exInfo._passNumber, frameIter.ControlPC, frameIter.SP); | |||
|
|||
UpdateStackTrace(exceptionObj, exInfo._frameIter.FramePointer, (IntPtr)frameIter.OriginalControlPC, frameIter.SP, ref isFirstRethrowFrame, ref prevFramePtr, ref isFirstFrame, ref exInfo); | |||
#if !NATIVEAOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want NATIVEAOT behavior to be different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually not sure if the issue occurs with NativeAOT too. It uses a different way of filtering frames in the UpdateStackTrace based on equal frame pointers, which doesn't work on coreclr. So I've made the change for coreclr, but plan to investigate the nativeaot case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the diagnostic test that this fixing do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the NestedException2 test. Basically, there is this code:
public static void Foo()
{
try
{
Console.WriteLine("Foo calling Bar");
ExceptionsTestHelper.Bar<int>(1);
}
catch (Exception inner)
{
throw new Exception("doh", inner);
}
}
}
The ExceptionsTestHelper.Bar<int>(1);
just throws InvalidOperationException
. The issue is that the Exception
thrown in the code above has the line of ExceptionsTestHelper.Bar<int>(1);
on the stack trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that the Exception thrown in the code above has the line of ExceptionsTestHelper.Bar(1); on the stack trace.
I see the My.Main()
line duplicated:
C:\runtime\artifacts\bin\coreclr\windows.x64.Release>corerun test.exe
Foo calling Bar
Unhandled exception. System.Exception: doh
---> System.InvalidOperationException: Operation is not valid due to the current state of the object.
at My.Bar[T](Int32 x)
at My.Main()
--- End of inner exception stack trace ---
at My.Main()
C:\runtime\artifacts\bin\coreclr\windows.x64.Release>set DOTNET_LegacyExceptionHandling=0
C:\runtime\artifacts\bin\coreclr\windows.x64.Release>corerun test.exe
Foo calling Bar
Unhandled exception. System.Exception: doh
---> System.InvalidOperationException: Operation is not valid due to the current state of the object.
at My.Bar[T](Int32 x)
at My.Main()
--- End of inner exception stack trace ---
at My.Main()
at My.Main()
There is ifdefed-out code in UpdateStackTrace that seems to be dealing with this situation:
runtime/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs
Lines 966 to 968 in 68f7885
#if NATIVEAOT | |
if ((prevFramePtr == UIntPtr.Zero) || (curFramePtr != prevFramePtr)) | |
#endif |
Can this bug be fixed by deleting the ifdef in UpdateStackTrace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's what I've mentioned above. The frame pointer check doesn't work in coreclr, since there are cases where two non-funclet methods have the same frame pointer - when they don't use Rbp as a frame pointer and use just Rsp instead. I've actually attempted to fix the problem by removing the ifdef, thinking I was originally wrong. But then another diagnostic test started to fail due the fact I've just mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to get this unified with native AOT
A recently added diagnostic test has revealed that throwing an exception from a funclet results in an extra frame on the stack trace that should not be there. This change fixes it in a manner equivalent to how the old EH handles that.
ab6093b
to
8d5fe81
Compare
8d5fe81
to
3d6f0fc
Compare
Merging as the test run with temporarily enabled new EH passed. |
A recently added diagnostic test has revealed that throwing an exception from a funclet results in an extra frame on the stack trace that should not be there. This change fixes it in a manner equivalent to how the old EH handles that.
A recently added diagnostic test has revealed that throwing an exception from a funclet results in an extra frame on the stack trace that should not be there.
This change fixes it in a manner equivalent to how the old EH handles that.